Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: allow to authenticate with an existing refresh token #804

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

krzysu
Copy link
Contributor

@krzysu krzysu commented Jan 23, 2024

No description provided.

Copy link

changeset-bot bot commented Jan 23, 2024

🦋 Changeset detected

Latest commit: fe707b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lens-protocol/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

height bot commented Jan 23, 2024

This pull request has been linked to 1 task:

💡Tip: Add "Close T-18486" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

Copy link

vercel bot commented Jan 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
lens-sdk-example-web-wagmi ✅ Ready (Inspect) Visit Preview Jan 24, 2024 0:55am

@@ -35,6 +35,11 @@ export class Authentication implements IAuthentication {
this.credentials = new CredentialsStorage(context.storage, context.environment.name);
}

async fromRefreshToken(refreshToken: string): Promise<void> {
const credentials = new Credentials(undefined, refreshToken);
await this.credentials.set(credentials);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cesarenaldi I made it as simple as this.
shall we validate if the input is a valid jwt token and if it is a lens refresh token or leave it out to the consumer? if not valid then the API will fail on the refresh step anyway and it will happen the first time you call any client method. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a simple approach works for now. I have a question about the design choice.

Why you went for a method compared to a configuration option in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first that client.authentication module contains all methods related to authentication, so it's easier to find it here if you are checking what is possible,
2nd, you might have a client instance from another place of your app and only authenticate later when you have a token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably not clear but each form of authentication will overwrite the previous one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see and understand the rationale. The only thing is that the method name might not be intelligible. the "fromSomething" kinda make sense as static factory method (e.g. Car.fromParts(...)) but not sure if works well as instance method of a submodule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to better ideas 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to

/**
* Authenticate with an existing, valid refresh token.
*/
authenticateWith({ refreshToken }: { refreshToken: string }): Promise<void>;

@@ -97,7 +97,7 @@
"typescript": "5.2.2"
},
"engines": {
"node": "^18.15.0"
"node": ">=18"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a range up to current stable (i.e. 20)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cesarenaldi
Copy link
Member

Hey @bigint, this change should enable you to use the LensClient for gated publication starting from the credentials you already have.

@bigint
Copy link

bigint commented Jan 24, 2024

Amazing @cesarenaldi when this is going live?

@cesarenaldi
Copy link
Member

Amazing @cesarenaldi when this is going live?

This week.

@krzysu krzysu force-pushed the T-18486/init-with-refresh-token branch from ff859f6 to fe707b7 Compare January 24, 2024 12:53
@krzysu krzysu changed the title client: allow to authenticate from existing refresh token client: allow to authenticate with an existing refresh token Jan 24, 2024
@krzysu krzysu merged commit fea579a into develop Jan 25, 2024
6 checks passed
@krzysu krzysu deleted the T-18486/init-with-refresh-token branch January 25, 2024 08:06
@cesarenaldi
Copy link
Member

Amazing @cesarenaldi when this is going live?

This week.

@bigint sorry, we had some last minute things to deal with which disrupted out SDK release. Aiming to get this out early next week.

@cesarenaldi
Copy link
Member

@bigint this is now released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants